-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(http-server): adds http/2 support #4989
Conversation
Great start @achrinza . Thank you. :) Do you have mocha tests in mind as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test cases. thx.
@emonddr That's the plan; though I'm trying to identify what would be suitable tests. |
Signed-off-by: Rifa Achrinza <[email protected]>
Signed-off-by: Rifa Achrinza <[email protected]>
Signed-off-by: Rifa Achrinza <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for starting the work on adding HTTP/2 support 👍
The way how I see http-server designed, it allows applications to use the same code to setup a listening HTTP/x server, and let http-server implementation to decide which Node.js API (http/https/http2) to use under the hood.
In that light, your proposal to support both RequestListener
and Http2RequestListener
does not make sense to me, because the application would have to understand whether it's using HTTP/1 or HTTP/2 in order to provide the right request handler.
Personally, I think we should support only the current RequestHandler
signature and use HTTP/2 Compatibility API to setup HTTP/2 servers. See the docs here: https://nodejs.org/dist/latest-v12.x/docs/api/http2.html#http2_compatibility_api
In the future, we may "upgrade" this package to require Http2RequestListener
only and and use forward-compatible wrapper around HTTP/1 to allow HTTP/2 handlers to deal with HTTP/1 requests. I believe such wrapper has been discussed in Node.js core in the past, but I don't know what was the outcome and whether it's actually possible to implement such wrapper at all.
In case we decide to continue with the direction you proposed originally, I have few comments to consider - see below.
@@ -81,7 +114,7 @@ export class HttpServer { | |||
* @param serverOptions | |||
*/ | |||
constructor( | |||
requestListener: RequestListener, | |||
requestListener: RequestListener | Http2RequestListener, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this constructor API sub-optimal, because it allows caller to mix HTTP v1 request listener with HTTP v2 server options and vice versa.
Can we use function overloads to implement more tight type checks please?
class HttpServer {
// ...
constructor(
requestListener: RequestListener,
serverOptions?: HttpOptions | HttpsOptions
);
constructor(
requestListener: Http2RequestListener,
serverOptions?: Http2Options | Https2Options
);
constructor(
requestListener: RequestListener | Http2RequestListener,
serverOptions?: HttpServerOptions,
) {
// the implementation
}
}
Thanks for the feedback @bajtos,
HTTP/2 isn't backwards-compatible unless we explicitly enable ALPN Negotiation. The main limiting factor is that this only supports HTTPS and will not support plain HTTP/1.
I have concerns that this would be limiting users from customizing certain properties such as the ALPN Negotiation. As an independent package, users should be able to use all features from the Node.js API (whether directly or indirectly), otherwise this would be a limiting factor.
Sounds like a good idea, though I'm not too sure on what the implications would be. WDYT? |
I am new to HTTP/2 and still learning the details. As I result, I did not fully understand the difference between different HTTP/2 server APIs and their type descriptions 🙈 I think most of my comments above can be ignored. Here is my current understanding of what options Node.js core provides:
Because a native HTTP/2 handler has very different signature, I think it's out of scope of Let's agree on what scenarios we want support, what scenarios we do not want to support, and what edge cases we need to consider. I see three types of handlers to consider:
And three kinds of server configurations:
That gives us 9 different scenarios. To keep things simpler, I am proposing for now to leave out Scenario A - HTTP/1 server and handler This is the current status. const server = new HttpServer(v1handler, {
protocol: 'http',
// ...
}); Scenario B - HTTP/1+2 server and HTTP/1 handler This will immediately enable all existing LoopBack applications to start accepting HTTP/2 clients, with no further changes necessary in const server = new HttpServer(v1handler, {
protocol: 'http2s',
allowHTTP1: true,
// ...
}); Scenario C - HTTP/1+2 server and HTTP/1+2 handler This is an upgrade that will eventually allow LB applications to start using HTTP/2 features if the client supports them, after we make the necessary changes in const server = new HttpServer(v1v2handler, {
protocol: 'http2s',
allowHTTP1: true,
// ...
}); Scenario D - HTTP/1 server and HTTP/1+2 handler This allows LB applications supporting HTTP/2 clients to be able to run within an HTTP/1 server. const server = new HttpServer(v1v2handler, {
protocol: 'http',
// ...
}); The important part in all of those scenarios above is that the handler signature remains the same independently of what server configuration is provided. We need to find a way how to write our type definitions to allow such usage patterns. @achrinza Is this in line with what you were thinking of when you started this pull request? I find it quite difficult to build the full context needed to provide a meaningful review. It would help me tremendously if we could split this pull request into smaller chunks. I am proposing the following baby steps, where each step provides a meaningful new feature that can be immediately used by our users. Let me start by illustrating the current status:
Step 1 Allow existing applications to start an HTTP/1+2 (and HTTP/2) server. See "Scenario B" for a test case.
This step can be done by the current pull request. I think it should be pretty straightforward, we just need to add new server-options type and verify that a HTTP/1 handler can be used with a HTTP/2 server. I think this is fully supported according to Node.js docs, but it seems like
Quoting from https://nodejs.org/api/http2.html#http2_compatibility_api
Step 2 Enable HTTP/1+2 handler signature. See "Scenario C" and "Scenario D" for test cases.
I am expecting that we will need to go deep into TypeScript language features and Step 2.5 Allow HTTP/2 only request handler signatures for servers that are not going to use Step 3 (longer term) Upgrade I am not sure how easy this is going to be, because we use Express under the hood and I don't know if Express supports HTTP/2 yet. There used to be major issues because of the way how Express patches the HTTP request & response objects and/or prototypes.
Step 4 (longer term) Update signatures of types used by applications (e.g.
That's how I am thinking about approaching HTTP/2 support. What do you think, @achrinza @emonddr @hacksparrow? |
export interface Https2Options | ||
extends BaseHttpOptions, | ||
http2.SecureServerOptions { | ||
protocol: 'https2'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: can we please use http2s
("http2" + "secure")? I find https2
weird to read.
protocol: 'https2'; | |
protocol: 'http2s'; |
Note that HTTP/2 spec puts the version number before additional flags too, e.g h2c
for clear-text connections (not hc2
). See e.g. https://stackoverflow.com/a/37373039/69868
Similarly the interface should be called Http2SecureOptions
or perhaps Http2sOptions
if you prefer.
How naive I was. Express still does not support HTTP/2, see expressjs/express#3730. Because we are internally using Express, it's likely that HTTP/2 will not work in LoopBack 4. I wish we picked a more modern framework as the base, e.g fastify 🤷♂️ |
Thank you @achrinza Good proposal 👍 |
We just switch the contribution method from CLA to DCO, making your contribution easier in the future. Please sign the commits with DCO by amending your commit messages with
Please refer to this docs page for details. Thanks! |
Closing this issue as it's not actionable. |
@achrinza , can you please re-open it, we need HTTP2 support in LB4 Is there any way to do it? or is there any future plan for supporting it? reference : https://strongloop.com/strongblog/serving-a-progressive-web-app-from-loopback/ |
Signed-off-by: Rifa Achrinza [email protected]
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈